feat: migrate track chair list and edit to mui components#918
Conversation
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughRefactors Track Chairs to a hooks-based MUI page, adds a Formik-based TrackChairDialog, updates track-chair action requests/error handling (adds member.id, passes meta, uses snackbarErrorHandler and .finally()), adjusts reducer reset/perPage behavior, enhances async select prop forwarding, and tweaks an i18n delete warning string. ChangesTrack Chairs UI Modernization & Error Handling
Sequence Diagram(s)sequenceDiagram
participant User
participant ListPage as TrackChairListPage
participant Dialog as TrackChairDialog
participant Actions as Redux Actions
participant API as Backend API
rect rgba(100, 150, 200, 0.5)
Note over User,API: Initial page load
ListPage->>Actions: getTrackChairs(meta: trackId, term, order, orderDir, perPage)
Actions->>API: GET /track-chairs?meta...
API-->>Actions: 200 + trackChairs (with member.id)
Actions->>ListPage: dispatch RECEIVE_TRACK_CHAIRS
end
rect rgba(150, 200, 150, 0.5)
Note over User,Dialog: Add new track chair
User->>ListPage: click Add button
ListPage->>Dialog: render with empty entity
User->>Dialog: select member, track(s), Save
Dialog->>Actions: addTrackChair(memberId, trackIds)
Actions->>API: POST /track-chairs
API-->>Actions: 201 + new chair
Actions->>ListPage: dispatch success
ListPage->>Actions: getTrackChairs (refresh)
ListPage->>Dialog: close (dialogEntity=null)
end
rect rgba(200, 150, 100, 0.5)
Note over User,Dialog: Edit existing track chair
User->>ListPage: click edit icon
ListPage->>Dialog: render with populated entity
User->>Dialog: modify tracks, Save
Dialog->>Actions: saveTrackChair(chairId, trackIds)
Actions->>API: PUT /track-chairs/{id}
API-->>Actions: 200 + updated chair
ListPage->>Actions: getTrackChairs (refresh)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/actions/track-chair-actions.js (1)
149-157:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways clear loading state on add failures.
This path still dispatches
stopLoading()only on success. IfpostRequestrejects, the global loading state stays active and the new dialog flow gets stuck after a failed create.Suggested fix
return postRequest( null, createAction(TRACK_CHAIR_ADDED), `${window.API_BASE_URL}/api/v1/summits/${currentSummit.id}/track-chairs`, { member_id: member.id, categories: trackIds }, snackbarErrorHandler - )(params)(dispatch).then(() => { + )(params)(dispatch).finally(() => { dispatch(stopLoading()); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/actions/track-chair-actions.js` around lines 149 - 157, The add-track-chair flow only calls dispatch(stopLoading()) in the success .then, leaving the global loading state active on postRequest rejection; update the returned promise chain that wraps postRequest(...)(params)(dispatch) so that dispatch(stopLoading()) runs regardless of outcome (use .finally or a .catch that rethrows after dispatching stopLoading()). Reference the postRequest call that uses createAction(TRACK_CHAIR_ADDED), snackbarErrorHandler, and ensure stopLoading is dispatched in the failure path as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/track_chairs/track-chair-list-page.js`:
- Around line 125-131: The current handleSave treats a changed member in edit
mode as an add, causing duplicate assignments; update handleSave to perform an
explicit replace instead of add when dialogEntity?.originalMemberId !==
member?.value and id exists: either call a replaceTrackChair(member.value, id,
trackIds) helper (preferred) or sequentially call removeTrackChair(id) then
addTrackChair({ id: member.value }, trackIds), and finally clear the dialog;
alternatively, if reassignment is not allowed make the member field read-only in
edit mode (in the dialog render) so the branch to addTrackChair never runs for
edits. Ensure you reference dialogEntity.originalMemberId, handleSave,
addTrackChair, saveTrackChair and remove/replace helper names when implementing
the change.
- Around line 57-63: The early return based on currentSummit.id happens before
hooks run and can cause hook-order errors; keep the declarations (e.g., const
[dialogEntity, setDialogEntity] = useState(null)) and the useEffect hook in
place, move the render guard that checks currentSummit.id below the hooks (so
hooks always register), and change the effect to depend on currentSummit?.id
(useEffect(() => { if (currentSummit?.id) getTrackChairs(); },
[currentSummit?.id])) so getTrackChairs runs when the summit arrives.
---
Outside diff comments:
In `@src/actions/track-chair-actions.js`:
- Around line 149-157: The add-track-chair flow only calls
dispatch(stopLoading()) in the success .then, leaving the global loading state
active on postRequest rejection; update the returned promise chain that wraps
postRequest(...)(params)(dispatch) so that dispatch(stopLoading()) runs
regardless of outcome (use .finally or a .catch that rethrows after dispatching
stopLoading()). Reference the postRequest call that uses
createAction(TRACK_CHAIR_ADDED), snackbarErrorHandler, and ensure stopLoading is
dispatched in the failure path as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4ec317b-b43b-40a7-8d38-7a99aa796a4b
📒 Files selected for processing (5)
src/actions/track-chair-actions.jssrc/i18n/en.jsonsrc/pages/track_chairs/components/track-chair-dialog.jssrc/pages/track_chairs/track-chair-list-page.jssrc/reducers/track_chairs/track-chair-list-reducer.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/track_chairs/track-chair-list-page.js (1)
57-63:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRuntime crash:
currentSummit.tracksaccessed before guard.Line 63 accesses
currentSummit.tracksbefore the guard at line 157. IfcurrentSummitis undefined, this crashes. Additionally, theuseEffecthas an empty dependency array, sogetTrackChairswon't be called when the summit loads after mount.Move
chairTracksderivation and the early return guard together, and add proper dependencies to the effect:Proposed fix
const [dialogEntity, setDialogEntity] = useState(null); + const chairTracks = currentSummit?.tracks?.filter((t) => t.chair_visible) ?? []; + useEffect(() => { - if (currentSummit) getTrackChairs(); - }, []); + if (currentSummit?.id) getTrackChairs(); + }, [currentSummit?.id]); - const chairTracks = currentSummit.tracks.filter((t) => t.chair_visible); + if (!currentSummit?.id) return <div />;Then remove the duplicate guard at line 157.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/track_chairs/track-chair-list-page.js` around lines 57 - 63, Move the derivation of chairTracks so it runs only after the early-return guard that checks currentSummit (i.e., return/loading when !currentSummit) to avoid accessing currentSummit.tracks when currentSummit is undefined; update the useEffect that calls getTrackChairs to include currentSummit and getTrackChairs in its dependency array (useEffect(() => { if (currentSummit) getTrackChairs(); }, [currentSummit, getTrackChairs])) so the fetch runs when the summit loads, and remove the now-duplicate early-return guard later in the component; references: chairTracks, currentSummit, getTrackChairs, useEffect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/pages/track_chairs/track-chair-list-page.js`:
- Around line 57-63: Move the derivation of chairTracks so it runs only after
the early-return guard that checks currentSummit (i.e., return/loading when
!currentSummit) to avoid accessing currentSummit.tracks when currentSummit is
undefined; update the useEffect that calls getTrackChairs to include
currentSummit and getTrackChairs in its dependency array (useEffect(() => { if
(currentSummit) getTrackChairs(); }, [currentSummit, getTrackChairs])) so the
fetch runs when the summit loads, and remove the now-duplicate early-return
guard later in the component; references: chairTracks, currentSummit,
getTrackChairs, useEffect.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b6b9930-5b7b-4fbd-8a77-4a9210e5efbe
📒 Files selected for processing (1)
src/pages/track_chairs/track-chair-list-page.js
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
| setIsSaving(true); | ||
| const newMember = dialogEntity?.originalMemberId !== member?.value; | ||
| const action = | ||
| !id || newMember |
There was a problem hiding this comment.
When editing a track chair and the user selects a different member, this branch calls addTrackChair (POST — new record) but never deletes the original. After save, both records appear in the list: the old one with member A and the new one with member B, both holding the same track assignments.
saveTrackChair only sends { categories: trackIds } — the PUT endpoint does not accept member_id — so changing the member via PUT is not supported. Two options:
- Call
deleteTrackChair(id)beforeaddTrackChairwhennewMemberis true. - Disable the member field in edit mode to match the original form behavior (which never allowed changing the member).
| exportTrackChairs | ||
| }) => { | ||
| const [dialogEntity, setDialogEntity] = useState(null); | ||
| const [isSaving, setIsSaving] = useState(false); |
There was a problem hiding this comment.
isSaving belongs inside TrackChairDialog, not here. This is the same deviation tracked in https://app.clickup.com/t/86bag2zk7 — now introduced in a new page.
The pattern (from summit-admin-popup-dialog-pattern.md and the reference impl in media-file-type-list-page.js):
Parent — returns the promise, triggers list reload, never touches isSaving:
const handleSave = ({ id, member, trackIds }) => {
const newMember = dialogEntity?.originalMemberId !== member?.value;
return (!id || newMember
? addTrackChair({ id: member.value }, trackIds)
: saveTrackChair(id, trackIds)
).then(() => getTrackChairs(trackId, term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir));
};Dialog — owns isSaving, closes itself on success, stays open on error:
const [isSaving, setIsSaving] = useState(false);
const handleOnSave = (values) => {
if (isSaving) return;
setIsSaving(true);
Promise.resolve(onSave(values))
.then(() => onClose())
.catch(() => {})
.finally(() => setIsSaving(false));
};Also remove the isSaving prop from <TrackChairDialog> — the dialog no longer receives it from outside.
| > | ||
| <Button | ||
| variant="outlined" | ||
| onClick={() => exportTrackChairs(trackChairs)} |
There was a problem hiding this comment.
exportTrackChairs is defined as () => async (dispatch, getState) => ... — no parameters. The trackChairs argument passed here is silently discarded; the action reads its own data from trackChairListState via getState(). Change to () => exportTrackChairs().
| onSave: PropTypes.func.isRequired | ||
| }; | ||
|
|
||
| export default TrackChairDialog; |
There was a problem hiding this comment.
No tests for this new component. Critical paths to cover:
- Validation fires when
memberortrackIdsis empty (submit should be blocked) onSavereceives{ id, member, trackIds }on valid submit- Save button disabled while
isSaving={true} - Close button and Escape key blocked while
isSaving={true}
The handleSave branching in the list page (addTrackChair vs saveTrackChair, and the missing delete-then-add path when the member changes) is also untested.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/mui/formik-inputs/mui-formik-async-select.js`:
- Line 124: The spread operator `{...rest}` is positioned after critical
internal props in the component's prop list, allowing consumer-provided props to
override essential handlers like `onChange`, `value`, `loading`, and `multiple`,
which breaks Formik form synchronization. Move the `{...rest}` spread to appear
before all critical internal props (such as `onChange`, `value`, `loading`,
`multiple`, `onBlur`, etc.) so that the internal props always take precedence
and cannot be overridden by consumer input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 156c349d-b0e9-4cba-b712-1f5bd86f29fe
📒 Files selected for processing (5)
src/components/mui/formik-inputs/mui-formik-async-select.jssrc/pages/track_chairs/__tests__/track-chair-list-page.test.jssrc/pages/track_chairs/components/__tests__/track-chair-dialog.test.jssrc/pages/track_chairs/components/track-chair-dialog.jssrc/pages/track_chairs/track-chair-list-page.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/pages/track_chairs/components/track-chair-dialog.js
- src/pages/track_chairs/track-chair-list-page.js
| {option.label} | ||
| </li> | ||
| )} | ||
| {...rest} |
There was a problem hiding this comment.
Risk: {...rest} can override critical internal props.
Spreading {...rest} after all other props means consumer-provided props like onChange, value, loading, or multiple can override the component's internal Formik-integrated handlers, breaking form synchronization silently.
Consider one of these safer patterns:
- Spread
{...rest}before critical props so internal props take precedence. - Explicitly allow only safe passthrough props (e.g.,
disabled,sx,className,testId).
🛡️ Recommended fix: spread rest before critical props
/>
)}
- {...rest}
+ disabled={disabled}
+ sx={sx}
+ className={className}
/>Or if you need full flexibility, spread before:
return (
<Autocomplete
+ {...rest}
options={options}
value={value}
onChange={handleChange}
loading={loading}
multiple={isMulti}
...
- {...rest}
/>
);Note: If spreading before, document which props consumers can safely override.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/mui/formik-inputs/mui-formik-async-select.js` at line 124, The
spread operator `{...rest}` is positioned after critical internal props in the
component's prop list, allowing consumer-provided props to override essential
handlers like `onChange`, `value`, `loading`, and `multiple`, which breaks
Formik form synchronization. Move the `{...rest}` spread to appear before all
critical internal props (such as `onChange`, `value`, `loading`, `multiple`,
`onBlur`, etc.) so that the internal props always take precedence and cannot be
overridden by consumer input.
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
2 participants
ref: https://app.clickup.com/t/86b9pc89t
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit